Implement graph binary protocol #217
Conversation
| Ok(Response { | ||
| request_id: Some(middle_form.request_id), | ||
| result: ResponseResult { | ||
| data: serializer_v2::deserializer_v2(&middle_form.result.data).map(Some)?, |
There was a problem hiding this comment.
This is a notable change. Previously the result.data would only be parsed depending on the status code. To normalize the disjoint (de)serializers I go ahead and attempt to parse it regardless, but handle its omission via a None in the normalized type.
|
Also looks like there's something up with the |
…the related cfg feature
…e intermingling and retry loops for merge tests that observe state between transactions to prevent flakey failures
|
@wolf4ood flipping this to ready since I've completed what I had left outstanding. Let me know what you think, as always happy to make changes. About a net negative 1k lines despite the graph binary protocol being added from parameterizing all the tests. This means any future protocol that gets added will just need to be added as an additional annotation in There was a few tests that were adjusted namely either skipping the older On the note of tests, the actual tests are passing but the |
|
As previously mentioned so long as people were using the default GraphSONV3 (de)serializer and not explicitly setting it I believe this should be a transparent change. I didn't change the default, just added the new protocol as an option after renaming the enum. Figured changing the default is something that is your discretion. |
.github/workflows/test.yml
Outdated
There was a problem hiding this comment.
Added derive flag to tests, notice the derive tests weren't being ran
| fn backwards_compatability_get(&self, key: GKey) -> Option<&GValue> { | ||
| //The GraphBinary protocol may be returning properties | ||
| //as the distinct "T" type (T::Id) whereas older protocols | ||
| //would have just sent them as GKey::String("id"), etc | ||
| //This function is intended as a fallback for backwards compatability that if a caller | ||
| //requests a get or try_get for "id" that we also then attempt it T::Id or one of its siblings | ||
| //This also maintains the representation needed for the derive crate to find | ||
| //the types since the derive crate can't discriminate between a vertex property called "id" | ||
| //or a T::Id since they're both just called "id" | ||
| match key { | ||
| GKey::String(string) => match TryInto::<T>::try_into(string.as_str()) { | ||
| Ok(fallback_key) => self.0.get(&fallback_key.into()), | ||
| Err(_) => None, | ||
| }, | ||
| _ => None, | ||
| } |
There was a problem hiding this comment.
@wolf4ood really wanted to call attention to this. Originally I was intended to just make users make the jump to calling a map.get(T::Id) vs map.get("id") but once I fixed the derive tests I hit a wall that the derive macro logic can't make that jump (I think?).
Namely the derive macro can't really know if the final struct should be using a property called "id" or "T::id" now that GraphBinary discriminates between the two. So I was hesitant to "fix" the problem on that side.
Doing it here means anyone who was doing map.get("id") will transparently get switched to map.get(T::id) under the covers if their initial get attempt returns a None now. That way it's only a perf hit as a fallback, whereas the derive macro would have had to do that for every property I think.
Anyways, wanted to call this out because it's kind of a catch 22 situation that we're forced into I think because of the derive macros.
…custom JanusGraphRelationIdentifier
| - { className: org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV1, config: { ioRegistries: [org.janusgraph.graphdb.tinkerpop.JanusGraphIoRegistry] }} | ||
| - { className: org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV1, config: { serializeResultToString: true }} |
There was a problem hiding this comment.
By default JanusGraph doesn't have GraphBinary enabled so the custom vertex id tests were failing as the serializer wasn't enabled. This config enables them here and the config is then mounted into the needed spot in the docker-compose file
|
Flipping back to draft. While integrating this branch into my application logic I found some placed JanusGraph does things a little bit differently. Also discovered edge properties aren't being serialized in either GraphSONV2 or GraphSONV3, so fixing that as well atm. |
…nted deserialization of Property in GraphBinary
|
K, back to ready. Completed trying it out with my application and at least for my purposes it all works. |
@wolf4ood
wanted to give you a heads up of the PR coming. If you get a chance please look at it while it's in this preliminary draft stage because I've made some significant overhauls and it'd be nice to get your feedback before I continue.It's no longer in draft.I'll annotate particular areas below 👍 .
But in addition to implementing the GraphBinaryV1 protocol this PR makes a few other changes:
integration_traversalandintegration_traversal_v2and parameterized it eventually replacing the original duo. This way the new GraphBinaryV1 protocol is equally tested as the two prior serializers by running the same tests. I did have to make tweaks to smooth small differences (namely differences in output key types in maps) and the V2 serializer doesn't appear to support some things, so its opted out where neededderiveflag to tests, it didn't appear they were previously being rangetandtry_getelaborated belowI intend to continue this next week, parameterizing the remaining test files so GraphBinary can piggyback on the tests that already exist 👍 .